feat(wait): implement AnyMultiStrategy: ForAny equivalent to ForAll.#3719
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds AnyMultiStrategy (ForAny) to concurrently run multiple wait.Strategy instances and succeed when any completes; includes configurable global deadline and startup-timeout defaults, nil/typed-nil filtering, walk traversal update, and tests for selection, fail‑fast behavior, deadline/startup‑timeout propagation, and nil handling. ChangesForAny Multi-Strategy Implementation
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AnyMultiStrategy
participant StrategyA
participant StrategyB
Caller->>AnyMultiStrategy: WaitUntilReady(ctx, target)
AnyMultiStrategy->>StrategyA: WaitUntilReady(ctx±deadline, target)
AnyMultiStrategy->>StrategyB: WaitUntilReady(ctx±deadline, target)
StrategyA-->>AnyMultiStrategy: error | nil (first completion)
AnyMultiStrategy-->>Caller: return (first result)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wait/any.go`:
- Around line 56-58: The String() implementation for AnyMultiStrategy returns
the wrong label when there are no strategies; in AnyMultiStrategy.String() check
the ms.Strategies empty branch and change the returned literal from "all of:
(none)" to "any of: (none)" so diagnostics correctly read "any of: (none)" when
ms.Strategies is empty.
- Around line 117-123: The ForAny loop currently returns on the first non-nil
error received from resCh, causing premature failure; change ForAny's result
handling so it returns immediately on the first nil (success) but only returns
an aggregate/error after all launched strategies have reported non-nil errors or
the context is done. Concretely: in the select reading from resCh, treat err ==
nil as immediate success (return nil); for non-nil errors, record the error (or
count failures) and decrement a remaining counter for the total started
strategies (use the same launch code that created resCh to determine N), and
only return an error when remaining reaches zero (or when ctx.Done() triggers);
ensure you still handle ctx.Done() as an early cancel path. Reference ForAny and
resCh to locate where to implement the counting/aggregation logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39c58d44-1158-40d1-b596-258d1ac79b15
📒 Files selected for processing (2)
wait/any.gowait/any_test.go
There was a problem hiding this comment.
Pull request overview
This PR adds a new composite wait strategy (ForAny) to the wait package, intended to be the “any-of” counterpart to the existing ForAll strategy (issue #3717), allowing container startup to proceed once any of multiple conditions is satisfied.
Changes:
- Introduces
AnyMultiStrategyand theForAny(...)constructor to wait on multiple strategies concurrently. - Adds unit tests covering basic behavior, deadlines, default timeouts, and nil strategies handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
wait/any.go |
Implements the new ForAny composite wait strategy and its configuration options. |
wait/any_test.go |
Adds tests validating ForAny behavior, deadline/timeout propagation, and nil-handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for this! I think that, once the bot comments are addressed, we can move on with the review and eventually merge, because I like this addition. I also miss the counterpart of this feature in Other than that, thanks again for contributing to the project! |
jeanbza
force-pushed
the
forany2
branch
3 times, most recently
from
d53bd24 to
56de82c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wait/walk.go (1)
62-70: ⚡ Quick winAdd
Walkparity tests for*AnyMultiStrategyremove/stop behavior.This new branch extends mutation traversal to
*AnyMultiStrategy, but currentwait/walk_test.gocoverage shown in context validatesErrVisitRemove/ErrVisitStopsemantics only through*MultiStrategy(ForAll). Add equivalent tests forForAnyto lock in this contract and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wait/walk.go` around lines 62 - 70, Add unit tests in wait/walk_test.go that mirror the existing ErrVisitRemove and ErrVisitStop semantics validated for *MultiStrategy (ForAll) but target *AnyMultiStrategy (ForAny); specifically, create test cases that build an AnyMultiStrategy with child strategies, run the Walk/walkAndMutate traversal and assert that returning ErrVisitRemove removes the current child and ErrVisitStop halts further visits for ForAny, ensuring behavior matches the ForAll tests and guarding against regressions in AnyMultiStrategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@wait/walk.go`:
- Around line 62-70: Add unit tests in wait/walk_test.go that mirror the
existing ErrVisitRemove and ErrVisitStop semantics validated for *MultiStrategy
(ForAll) but target *AnyMultiStrategy (ForAny); specifically, create test cases
that build an AnyMultiStrategy with child strategies, run the Walk/walkAndMutate
traversal and assert that returning ErrVisitRemove removes the current child and
ErrVisitStop halts further visits for ForAny, ensuring behavior matches the
ForAll tests and guarding against regressions in AnyMultiStrategy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75eed78f-26e9-4957-8932-1d4dac696bcb
📒 Files selected for processing (3)
wait/any.gowait/any_test.gowait/walk.go
🚧 Files skipped from review as they are similar to previous changes (1)
- wait/any.go
Done (docs) |
jeanbza
force-pushed
the
forany2
branch
4 times, most recently
from
33bc02b to
896e238
Compare
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM, thanks!
I'll merge it as soon as the CI is green, thank you for your hard work contributing to the project! 💪
|
@jeanbza the lint phase failed, could you run |
Done |
|
@jeanbza I added one commit with the lint fix, once the CI is green, we are good to go! |
Signed-off-by: mdelapenya <mdelapenya@gmail.com>
|
Merged, thanks for your hard work improving the library! 👏 |
* main: (24 commits) feat(wait): implement AnyMultiStrategy: ForAny equivalent to ForAll. (testcontainers#3719) chore(wait)!: change url callback in wait.ForSQL to accept network.Port (testcontainers#3650) chore(deps): bump github.com/shirou/gopsutil/v4 from 4.26.4 to 4.26.5 (testcontainers#3713) chore(deps): bump golang.org/x/sys from 0.44.0 to 0.45.0 (testcontainers#3712) chore(deps): bump mkdocs-include-markdown-plugin from 7.2.2 to 7.3.0 (testcontainers#3711) chore(deps): bump slackapi/slack-github-action from 2.1.1 to 3.0.3 (testcontainers#3677) chore(deps): bump idna from 3.11 to 3.15 (testcontainers#3708) chore(deps): bump github.com/containerd/containerd/v2 (testcontainers#3709) feat(eventhubs): add WithAzuriteContainer and functional-options config builder (testcontainers#3722) fix(security): remove debug code that leaks Docker credentials (testcontainers#3721) fix(ollama): update test log (testcontainers#3715) chore(metrics): update usage metrics (2026-06-08) (testcontainers#3714) feat!: add PullImageWithPlatform to DockerProvider (testcontainers#3710) chore(deps): bump urllib3 from 2.6.3 to 2.7.0 (testcontainers#3704) chore(deps): bump github.com/shirou/gopsutil/v4 from 4.26.3 to 4.26.4 (testcontainers#3667) chore(deps): bump github.com/moby/moby/api from 1.54.1 to 1.54.2 (testcontainers#3676) chore(deps): bump golang.org/x/crypto from 0.48.0 to 0.51.0 (testcontainers#3689) chore(deps): bump google.golang.org/grpc in /modules/gcloud (testcontainers#3690) chore(deps): bump google.golang.org/grpc in /modules/dex (testcontainers#3686) feat(modules/dex): add Dex OIDC provider module (testcontainers#3659) ...
|
Thank you for the help delivering this. 🙏 |
What does this PR do?
Implements ForAny.
Why is it important?
It's the other side of ForAll.
Related issues
Fixes #3717.